Fix integer overflow in ecmult_multi_var when n is large #568
Conversation
src/ecmult_impl.h
Outdated
| if (n > (size_t)-1 - (max_points-1)) { | ||
| return 0; | ||
| } | ||
| *n_batches = (n + max_points-1) / max_points; |
There was a problem hiding this comment.
| *n_batches = (n + max_points-1) / max_points; | |
| *n_batches = (n + (max_points-1)) / max_points; |
you could also CHECK/VERIFY that *n_batches != 0 here. As I understand it, this can only happen if n == 0 and then this function won't be called currently.
src/ecmult_impl.h
Outdated
| if (n > (size_t)-1 - (*n_batches-1)) { | ||
| return 0; | ||
| } | ||
| *n_batch_points = (n + *n_batches-1) / *n_batches; |
There was a problem hiding this comment.
| *n_batch_points = (n + *n_batches-1) / *n_batches; | |
| *n_batch_points = (n + (*n_batches-1)) / *n_batches; |
|
ACK |
|
@real-or-random I added a commit |
src/ecmult_impl.h
Outdated
| static int secp256k1_ecmult_multi_batch_size_helper(size_t *n_batches, size_t *n_batch_points, size_t max_points, size_t n) { | ||
| if (max_points == 0) { | ||
| return 0; | ||
| } else if (max_points > ECMULT_MAX_POINTS_PER_BATCH) { |
|
Oh I didn't want to bother you with nits only. I assumed that the missing parentheses are real bugs but no, we deal with unsigned integers. :) |
jonasnick
force-pushed
the
fix-ecmult-multi-overflow
branch
2 times, most recently
from
90ff155 to
24908be
Compare
|
Removed unnecessary else (h/t @dcousens) and squashed |
src/ecmult_impl.h
Outdated
| max_points = ECMULT_MAX_POINTS_PER_BATCH; | ||
| } | ||
| /* Check overflow */ | ||
| if (n > (size_t)-1 - (max_points-1)) { |
There was a problem hiding this comment.
Unless we have a reason, I think we'd prefer to use stdint SIZE_MAX here. (and in all the other cases where the largest SIZE_T is required)
There was a problem hiding this comment.
SIZE_MAX requires C99 as far as I remember.
There was a problem hiding this comment.
@real-or-random It is an stdint.h type, like uint32_t. The codebase uses stdint.h ( https://github.com/bitcoin-core/secp256k1/blob/master/src/util.h#L15) types everywhere. In practice every compiler someone would want to use has this header (even non C99 compilers), but if someone did have a compiler without it, they'd have to provide a compatibility header.
There was a problem hiding this comment.
Okay, I see. It's equivalent but yes, SIZE_MAX is obviously much nicer to read. I was the one who suggested size_t(-1) to @jonasnick in the Schnorr signature PR: #558 (review) Maybe that's why @apoelstra used it in #557 too...
jonasnick
force-pushed
the
fix-ecmult-multi-overflow
branch
from
24908be to
c17ef67
Compare
|
Replaced |
|
ACK |
|
This seems fine, though you could also avoid the overflow checks entirely with n_batches = 1 + (n-1)/max_points;
n_batch_points = 1 + (n-1)/n_batches;This should work in all cases since |
jonasnick
force-pushed
the
fix-ecmult-multi-overflow
branch
from
c17ef67 to
c37e53d
Compare
|
I rewrote this PR with @jimpo's suggestion. I think this is better because the previous code was complex enough that we missed a potential bug [0] and the new code doesn't have unnecessary return 0's. Because [0] |
|
ACK |
|
utACK |
|
utACK c37e53d, changes look correct to me |
|
The addition of the trivial ecmult_multi algorithm in pr580 unfortunately created a trivial merge conflict for this PR, sorry about that. I am ACK on the PR, but need the rebase. |
|
If you'd prefer I can make a fixup commit, whatever you'd be happiest with. |
jonasnick
force-pushed
the
fix-ecmult-multi-overflow
branch
from
c37e53d to
2277af5
Compare
2277af5 Fix integer overflow in ecmult_multi_var when n is large (Jonas Nick) Pull request description: Without this PR ecmult_multi could return wrong results. If the number of points `n` is large enough then some or all multiplications could be skipped or the function could end up in an infinite loop. This PR adds two checks to prevent `n` from wrapping around. Tree-SHA512: 342944369b24776fa3ec0694eee159259ff67e94d2d8176c1d3159875f387d943d5bfdff7cde59f058e13f07fd09bde1cbc609426e63c8a5b8040e382dd865d8
|
ACK |
Without this PR ecmult_multi could return wrong results. If the number of points
nis large enough then some or all multiplications could be skipped or the function could end up in an infinite loop. This PR adds two checks to preventnfrom wrapping around.